Skip to content

[AIGTWY-4901] Only remove a Codex model ug wrote - #921

Open
david-siqi-liu wants to merge 1 commit into
david/aigtwy-4901-1-provenancefrom
david/aigtwy-4901-2-codex
Open

david-siqi-liu wants to merge 1 commit into
david/aigtwy-4901-1-provenancefrom
david/aigtwy-4901-2-codex

Conversation

@david-siqi-liu

@david-siqi-liu david-siqi-liu commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Codex's writer removed model and model_reasoning_effort from ~/.codex/ucode.config.toml and /etc/codex/managed_config.toml whenever ug had no managed default to pin, on every configure, launch, and state load.

Before / after

  • Before: ug removed the Codex model (and model_reasoning_effort) by name whenever it had no managed default, so a model the developer or an admin chose was deleted.

  • After: ug removes or restores model only when the destination file's provenance record shows ug wrote the current value. A developer-set model survives, model_reasoning_effort is never touched, and a model ug pinned for a prior workspace or managed default is still retired once the file has a record (or, for the managed file, a last-applied snapshot).

  • model is removed, or restored to its pre-ug value, only when the destination file's provenance record shows ug wrote the current value and nobody changed it since. A model ug pinned for a previous workspace or managed default is still retired, on configure and on launch.

  • model_reasoning_effort is never removed; ug never writes it.

  • default_model() only reads (that part already landed on its own in [AIGTWY-4881] Keep Codex's managed default model when ug saves state #1010). Launch still retires a model ug pinned, and forgets ownership of a model the developer has since changed.

  • Ownership is recorded after a confirmed write only. A value either file already held is not adopted unless ug's record (or, for the managed file before the record existed, its last-applied snapshot) shows ug wrote it, a managed config that was already up to date adopts nothing new, and ug no longer backs up its own generated file as if it predated ug after a workspace switch.

  • For a managed config ug wrote before the record existed, ownership of model is bootstrapped from ug's last-applied snapshot, so an upgrading user's stale pin is still retired. A private config without a record keeps any model it has.

  • Smart routing keeps its existing behavior.

This pull request and its description were written by Isaac.

@david-siqi-liu
david-siqi-liu added this pull request to stack #924 October 1, 2026 02:13
@david-siqi-liu
david-siqi-liu removed this pull request from stack #924 October 1, 2026 02:14
@david-siqi-liu
david-siqi-liu force-pushed the david/aigtwy-4901-2-codex branch from b567051 to b776f2f Compare October 1, 2026 02:28
@david-siqi-liu
david-siqi-liu added this pull request to stack #926 October 1, 2026 02:29
@david-siqi-liu david-siqi-liu changed the title Only remove a Codex model ug wrote [AIGTWY-4901] Only remove a Codex model ug wrote Oct 1, 2026
@david-siqi-liu
david-siqi-liu force-pushed the david/aigtwy-4901-2-codex branch from b776f2f to d79db10 Compare October 1, 2026 14:08
@david-siqi-liu
david-siqi-liu force-pushed the david/aigtwy-4901-2-codex branch from d79db10 to 1d37724 Compare October 1, 2026 14:24
@david-siqi-liu
david-siqi-liu force-pushed the david/aigtwy-4901-2-codex branch from 1d37724 to efc5f90 Compare October 1, 2026 15:49
@david-siqi-liu david-siqi-liu reopened this Oct 2, 2026
@david-siqi-liu
david-siqi-liu force-pushed the david/aigtwy-4901-2-codex branch from efc5f90 to 4ff9bec Compare October 2, 2026 13:31
@david-siqi-liu
david-siqi-liu force-pushed the david/aigtwy-4901-2-codex branch from 4ff9bec to 6f3221d Compare October 2, 2026 17:48
david-siqi-liu added a commit that referenced this pull request Oct 6, 2026
@david-siqi-liu
david-siqi-liu force-pushed the david/aigtwy-4901-2-codex branch 2 times, most recently from 725c829 to dbbd42a Compare October 6, 2026 02:13
@david-siqi-liu david-siqi-liu added the ug-review Run the automated UG review label Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

UG review

Reviewed Codex model ownership across configure, launch, and migration from managed-file snapshots. The migration can mistake a preserved admin model for a ug-owned pin.

Blocker

  • Avoid adopting inherited models from snapshots — src/ucode/agents/codex.py:651: can we distinguish generated pins from inherited snapshot values? an admin model added after the original backup and preserved by smart routing will be deleted when routing is disabled.

Automated advisory review of 1912551900f9 using Lilly's UG review rubric. It does not approve or block this PR.

@david-siqi-liu
david-siqi-liu force-pushed the david/aigtwy-4901-2-codex branch from 7710c24 to d4a33a5 Compare October 6, 2026 17:14
@david-siqi-liu
david-siqi-liu force-pushed the david/aigtwy-4901-2-codex branch from d4a33a5 to e7733f2 Compare October 6, 2026 17:44
@david-siqi-liu

Copy link
Copy Markdown
Collaborator Author

Blocker (adopting a matching developer model on the first recorded write): fixed in e7733f2. The private-file write now always compares against the file as it was before the write, so a model the developer already had is never claimed just because it equals the managed default, and stays when the default is removed. Added test_first_recorded_write_does_not_adopt_a_matching_developer_model, which fails on the old code. (The managed file already worked this way.)

Codex's writer removed `model` and `model_reasoning_effort` from the private
and OS-managed configs whenever ug had no managed default to pin, so a model
the developer, an admin or another tool chose was deleted on every configure
and launch.

- Removals of `model` go through the per-file provenance record: ug restores
  the pre-ug value or removes it only when it wrote the current value and
  nobody changed it since. The record is updated only after a confirmed write.
- `model_reasoning_effort` is never removed; ug never writes it.
- `default_model` no longer clears preferences, so loading or saving state
  never rewrites the config. Launch still retires a model ug pinned.

Co-authored-by: Isaac <no-reply@databricks.com>
@david-siqi-liu
david-siqi-liu force-pushed the david/aigtwy-4901-2-codex branch from e7733f2 to 1912551 Compare October 6, 2026 21:13
@david-siqi-liu

Copy link
Copy Markdown
Collaborator Author

Blocker (bootstrap adopting an admin model preserved in the last-applied snapshot): leaving as is. This only applies to the one-time upgrade bootstrap of a managed file that has no provenance record yet. Today's main removes the Codex model by name on every run whenever there is no managed default and smart routing is off, in both files, so that admin model is already deleted on main. With this PR it can happen at most once, on the first upgraded run, and never again once the record exists. Dropping the bootstrap would instead strand every upgrader's own stale ug pin, which is the reason the bootstrap exists.

sunishsheth2009 pushed a commit to sunishsheth2009/ucode that referenced this pull request Oct 6, 2026
…atabricks#1010)

Standalone extract of the `default_model` part of databricks#921, so CUJ 6 (databricks#987)
isn't blocked on the provenance stack.

`codex.default_model()` called `clear_model_preferences` when there was
no managed default in the state it was given. `ug configure` writes
Codex's managed `model`, then `save_state` -> `hydrate_state` (without
the managed overlay) -> `default_model_for_tool` ->
`codex.default_model()` deleted it again. Codex then started on whatever
model the gateway listed first instead of the admin's default. The live
CUJ 6 run reproduced this: with a UC-location config, Codex served
`codex_extra` instead of the managed default `codex_primary`.

Changes:
- `src/ucode/agents/codex.py`: `default_model()` no longer calls
`clear_model_preferences`, so looking up the default never rewrites
config. The launch path still calls it, so a stale model ug pinned is
still retired at launch.
- `tests/test_agent_codex.py`: the test that asserted the lookup cleared
`model` and `model_reasoning_effort` now asserts it leaves the config
byte-for-byte unchanged (the same flip databricks#921 makes).

databricks#921 still carries the broader provenance-based fix. When it lands, it
supersedes this line with no conflict in intent.

Validation: `ruff check` and `ruff format --check` pass. Full `pytest`:
3201 passed; the only 2 failures are `test_e2e_user_agent`, which needs
a live gateway and also fails locally on main. The updated test fails
without the fix.

This pull request and its description were written by Isaac.

Co-authored-by: Isaac <no-reply@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ug-review Run the automated UG review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant